add marion's adjustment to pvwatts_dc #2569
Conversation
pvlib/pvsystem.py
Outdated
| For example, a 500 W module that produces 95 W at 200 W/m^2 (a 5% relative | ||
| reduction in efficiency) would have a value of `k` = 0.01. | ||
There was a problem hiding this comment.
Please include Equation from above with k, I'd place it here.
There was a problem hiding this comment.
See my latest change. Is that what you had in mind?
There was a problem hiding this comment.
Yeah, that comment was not clear. I was thinking we should copy the equation for P_DC here, and show how k modifies the power. But the equation for the adjustment is not simple (the piecewise part), so I retract that thought. If someone wants to know how the adjustment works, we've provided code and the reference.
Co-authored-by: Cliff Hansen <[email protected]>
Co-authored-by: Cliff Hansen <[email protected]>
Co-authored-by: Cliff Hansen <[email protected]>
Co-authored-by: Cliff Hansen <[email protected]>
Co-authored-by: Cliff Hansen <[email protected]>
There was a problem hiding this comment.
Nice work @williamhobbs. Just a few simple comments/suggestions on the docs from me. Only had a quick look at the python implementation, LGTM at a glance but I could have a deeper look later if required
doi sphinx Co-authored-by: RDaxini <[email protected]>
Co-authored-by: RDaxini <[email protected]>
Co-authored-by: RDaxini <[email protected]>
pvlib/pvsystem.py
Outdated
| # apply Marion's correction if k is anything but zero | ||
| if k is not None: | ||
| err_1 = (k * (1 - (1 - effective_irradiance / 200)**4) / | ||
| (effective_irradiance / 1000)) | ||
| err_2 = (k * (1000 - effective_irradiance) / (1000 - 200)) | ||
|
|
||
| pdc_marion = np.where(effective_irradiance <= 200, | ||
| pdc * (1 - err_1), | ||
| pdc * (1 - err_2)) | ||
|
|
||
| # "cap" Marion's correction at 1000 W/m^2 | ||
| if cap_adjustment is True: | ||
| pdc_marion = np.where(effective_irradiance >= 1000, | ||
| pdc, | ||
| pdc_marion) | ||
|
|
||
| pdc = pdc_marion | ||
|
|
There was a problem hiding this comment.
@williamhobbs have you started / do you intend to write tests? The codecov check fails since this section is not covered by tests. Happy to help with that if you need.
There was a problem hiding this comment.
Haven't started, but figured I would need to. Do you have suggestions on tests to add? I have pretty limited experience with the pvlib testing structure/best practices.
There was a problem hiding this comment.
I'd hand-calculate half a dozen points, using k. Test for correct output with input of three types: float, array, Series. Then test with cap_adjustment=True.
There was a problem hiding this comment.
Be creative, test whatever comes to mind! Generally speaking, test a range of intended and extreme conditions.
Intended:
For the if statements, check whether the indented block is executed if the condition is met (for example, if k is not None, use some example values to check whether the correction is applied).
Whether the block is executed correctly should also be checked--- if k is not None, then check example irradiance values >200 and <=200 (is the intended behaviour executed correctly in these cases?)
Extreme:
What if the user enters non-physical values, how should these be handled and are they handled in this way? e.g. negative or NaN irradiance values
Someone else might be able to offer a clearer/more succinct explanation 😅
There was a problem hiding this comment.
I'd hand-calculate half a dozen points, using
k. Test for correct output with input of three types: float, array, Series. Then test withcap_adjustment=True.
Good point, check whether different data types are handled appropriately too
There was a problem hiding this comment.
@williamhobbs you do not have to test with k=None, that is covered by existing tests. Any new tests would be better in their own function, e.g., test_pvwatts_dc_with_k.
There was a problem hiding this comment.
@williamhobbs you do not have to test with
k=None, that is covered by existing tests.
Correct, my bad. I was trying to make a general point but overlooked that in the example.
See the codecov report
Any new tests would be better in their own function, e.g.,
test_pvwatts_dc_with_k.
Add to test_pvsystem.py (examples also visible there)
There was a problem hiding this comment.
@williamhobbs these look good to me! I see tests covering different dtypes, various combinations of k and cap_adjustment, etc. Get a second opinion from @cwhanse in case I am missing something.
One other case to add would be the scalar case when cap_adjustment=True and effective_irradiance>=1000
That should cover the else branch highlighted as missing in the codecov report. Something simple like:
def test_pvwatts_dc_cap_adjustment_scalar_above_1000():
irrad = 1200
temp_cell = 25
k = 0.01
cap_adjustment = True
out = pvsystem.pvwatts_dc(irrad, temp_cell, 100, -0.003, 25, k, cap_adjustment)
expected = 120.0
assert_allclose(out, expected)
So in this case, the unadjusted pdc value is returned. Right?
There was a problem hiding this comment.
Thanks @williamhobbs good stuff! Happy to see this added to pvlib, less sure about the API right now.
We've been talking about changing this function name to pvwatts_dc_v5 in #1350. I think these new parameters will look quite out of place if we do go through with that. Which makes me think a separate function might be preferable.
I'm not dismissing your point. But to reply, PVWatts v8 (or whatever it is called now) is a moving target that does not have much documentation. I think, if we do anything with the newer PVWatts in pvlib (once it stablizes and is documented somehow), we could name that new function "pvwatts_v8" and leave the existing functions names without the version suffix. |
Co-authored-by: Will Holmgren <[email protected]>
There was a problem hiding this comment.
Just a few formatting suggestions + a suggestion for the tests to complement my other comment #2569 (comment)
| 25, k, cap_adjustment)) | ||
| assert_allclose(out, expected) | ||
|
|
||
|
|
There was a problem hiding this comment.
See #2569 (comment); I think this test should help. The rest look good already, nice job!
| def test_pvwatts_dc_cap_adjustment_scalar_above_1000(): | |
| irrad = 1200 | |
| temp_cell = 25 | |
| k = 0.01 | |
| cap_adjustment = True | |
| out = pvsystem.pvwatts_dc(irrad, temp_cell, 100, -0.003, 25, k, cap_adjustment) | |
| expected = 120.0 | |
| assert_allclose(out, expected) |
There was a problem hiding this comment.
See my changes to test_pvwatts_dc_with_k_and_cap_adjustment(). It now includes irradiance > 1000 and cap_adjustment=True. I think I made those changes before seeing your comments here.
Does that cover it?
Co-authored-by: RDaxini <[email protected]>
Co-authored-by: RDaxini <[email protected]>
Co-authored-by: RDaxini <[email protected]>
|
A general comment: after other Will H. pointed out the issue with returning the same object type that was input, my code has gotten a lot harder to follow, with lots of nested ifs, slicing, etc. If anyone has suggestions for cleaning it up, I'm very interested. A few specific thoughts/questions:
Example of reorganizing the ifs. Is this better? (edit: I went ahead and made this change. Looks much better to me.) (2nd edit: implemented new changes suggested by Kevin, not shown in this comment.) click to expand and see code # apply Marion's correction if k is anything but zero
if k is not None:
err_1 = k * (1 - (1 - effective_irradiance / 200)**4)
err_2 = k * (1000 - effective_irradiance) / (1000 - 200)
# if input is Series or array
if hasattr(effective_irradiance, '__len__'):
# precalculate pdc before error adjustments
pdc_marion = (effective_irradiance * 0.001 * pdc0 *
(1 + gamma_pdc * (temp_cell - temp_ref)))
# apply error adjustments
pdc_marion[effective_irradiance <= 200] = (
pdc[effective_irradiance <= 200] -
(pdc0 * err_1[effective_irradiance <= 200]))
pdc_marion[effective_irradiance > 200] = (
pdc[effective_irradiance > 200] -
(pdc0 * err_2[effective_irradiance > 200]))
# "cap" Marion's correction at 1000 W/m^2
if cap_adjustment:
pdc_marion[effective_irradiance >= 1000] = (
pdc[effective_irradiance >= 1000])
# set negative power to zero
pdc_marion[pdc_marion < 0] = 0
# else (input is scalar)
else:
# apply error adjustments
if effective_irradiance <= 200:
pdc_marion = pdc - (pdc0 * err_1)
elif effective_irradiance > 200:
pdc_marion = pdc - (pdc0 * err_2)
# "cap" Marion's correction at 1000 W/m^2
if cap_adjustment:
if effective_irradiance >= 1000:
pdc_marion = pdc
# set negative power to zero
if pdc_marion < 0:
pdc_marion = 0
pdc = pdc_marion |
Having different computation paths for different input types results in quite a lot of code, and duplicating the math also introduces the risk of accidentally performing different computations for different input types. What we do elsewhere in pvlib is to have only one computation path that operates on numpy arrays (the lingua franca) and then fix up the types at the very end. Something like this (please check that the computations are correct; this is just for illustration): def pvwatts_dc(...):
pdc = (effective_irradiance * 0.001 * pdc0 *
(1 + gamma_pdc * (temp_cell - temp_ref)))
# apply Marion's correction if k is anything but zero
if k is not None:
# preserve input types
index = pdc.index if isinstance(pdc, pd.Series) else None
is_scalar = np.isscalar(pdc)
# calculate error adjustments
err_1 = k * (1 - (1 - effective_irradiance / 200)**4)
err_2 = k * (1000 - effective_irradiance) / (1000 - 200)
err = np.where(effective_irradiance <= 200, err_1, err_2)
if cap_adjustment:
err = np.where(effective_irradiance >= 1000, 0, err)
pdc = pdc - pdc0 * err
if index is not None:
pdc = pd.Series(pdc, index=index)
elif is_scalar:
pdc = float(pdc)
return pdc |
Thanks, @kandersolar! This is the exact help I needed. |
|
|
||
|
|
||
| def test_pvwatts_dc_with_k_and_cap_adjustment(): | ||
| irrad_trans = [100, 1200] | ||
| temp_cell = 25 | ||
| ks = [0.01, 0.15] | ||
| cap_adjustments = [False, True] | ||
| out = [] | ||
| expected = [9.0625, 120.25, 0, 123.75, 9.0625, 120.0, 0, 120.0] | ||
| for cap_adjustment in cap_adjustments: | ||
| for k in ks: | ||
| for irrad in irrad_trans: | ||
| out.append(pvsystem.pvwatts_dc(irrad, temp_cell, 100, -0.003, | ||
| 25, k, cap_adjustment)) | ||
| assert_allclose(out, expected) | ||
|
|
There was a problem hiding this comment.
I think this test can be greatly simplified... What exactly is the intention here? Testing with both k and cap adjustment is supplied? Why do we need two k values for that? I think the default cap_adjustment case is already covered in other tests so not sure why it's needed here.
There was a problem hiding this comment.
maybe I am missing something in https://github.com/pvlib/pvlib-python/pull/2569/files#r2440448406?
There was a problem hiding this comment.
I was going for a range of irradiance with and without and cap_adjustment, and a range of k values including a k and irrad combination that would have returned negative power without the pdc = np.where(pdc < 0, 0, pdc) line.
Let me know what you think about 86eb06e.
Co-authored-by: Cliff Hansen <[email protected]>
Co-authored-by: Cliff Hansen <[email protected]>
Co-authored-by: Cliff Hansen <[email protected]>
5 participants
[ ] Updates entries indocs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.Still some work to do . Replaces #2568.